butane/util: use yaml instead of json for unmarhal by angelcerveraroldan · Pull Request #2310 · coreos/ignition

angelcerveraroldan · 2026-08-27T14:00:00Z

Since yaml is a superset of json, we can use the yaml unmarshal function instead of the json one.

This unmarshalling will be more accurate, as it does not convert numbers to floats (which would lead to awkward formatting of the generated MCO)

Adding test and release note is still needed

closes: #2309

Since yaml is a super-set of json, we can use the yaml unmarshal function instead of the json one. This unmarshalling will be more accurate, as it does not convert numbers to floats (which would lead to awkward formatting of the generated MCO) closes: coreos#2309

coderabbitai · 2026-08-27T14:00:48Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 47c4297f-4083-45d4-98c8-c81e7309a9e3

📥 Commits

Reviewing files that changed from the base of the PR and between ade39b4 and f728981.

📒 Files selected for processing (1)

butane/config/util/util.go

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📜 Recent review details

⏰ Context from checks skipped due to timeout. (3)

GitHub Check: Test (1.25.x)
GitHub Check: Test (1.26.x)
GitHub Check: tmt-tests

⚠️ CI failures not shown inline (2)

GitHub Actions: Release notes / 0_Require release note.txt: butane/util: use yaml instead of json for unmarhal

Conclusion: failure

View job details

##[group]Run set -euo pipefail
 �[36;1mset -euo pipefail�[0m
 �[36;1mif [ -n "skip-notes" ]; then�[0m
 �[36;1m    # Don't trust the label list in the event metadata, since runs�[0m
 �[36;1m    # can be scheduled out of order and the list might be stale.�[0m
 �[36;1m    label=$(curl --no-progress-meter \�[0m
 �[36;1m        -H "Accept: application/vnd.github+json" \�[0m
 �[36;1m        -H "Authorization: token ***" \�[0m
 �[36;1m        "https://api.github.com/repos/coreos/ignition/pulls/2310" |�[0m
 �[36;1m        jq '.labels[] | select(.name == "skip-notes")')�[0m
 �[36;1m    if [ -n "${label}" ]; then�[0m
 �[36;1m        echo "PR has skip-notes label; skipping"�[0m
 �[36;1m        exit 0�[0m
 �[36;1m    fi�[0m
 �[36;1mfi�[0m
 �[36;1mdiffinfo=$(curl --no-progress-meter \�[0m
 �[36;1m    -H "Accept: application/vnd.github+json" \�[0m
 �[36;1m    -H "Authorization: token ***" \�[0m
 �[36;1m    "https://api.github.com/repos/coreos/ignition/compare/ade39b435a73689a3a854037585d9dcb913fc86c...f72898152cef4e88f9aeeacbd487b6e527f12d22" |�[0m
 �[36;1m    jq '.files[] | select(.filename == "docs/release-notes.md")')�[0m
 �[36;1mif [ -z "${diffinfo}" ]; then�[0m
 �[36;1m    echo "Found no changes to docs/release-notes.md."�[0m
 �[36;1m    if [ -n "skip-notes" ]; then�[0m
 �[36;1m        echo "To ignore, add skip-notes label to PR."�[0m
 �[36;1m    fi�[0m
 �[36;1m    exit 1�[0m
 �[36;1mfi�[0m
 �[36;1mecho "Found change to docs/release-notes.md."�[0m
 shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
 ##[endgroup]
 Found no changes to docs/release-notes.md.
 To ignore, add skip-notes label to PR.
 ##[error]Process completed with exit code 1.

GitHub Actions: Release notes / Require release note: butane/util: use yaml instead of json for unmarhal

Conclusion: failure

View job details

##[group]Run set -euo pipefail
 �[36;1mset -euo pipefail�[0m
 �[36;1mif [ -n "skip-notes" ]; then�[0m
 �[36;1m    # Don't trust the label list in the event metadata, since runs�[0m
 �[36;1m    # can be scheduled out of order and the list might be stale.�[0m
 �[36;1m    label=$(curl --no-progress-meter \�[0m
 �[36;1m        -H "Accept: application/vnd.github+json" \�[0m
 �[36;1m        -H "Authorization: token ***" \�[0m
 �[36;1m        "https://api.github.com/repos/coreos/ignition/pulls/2310" |�[0m
 �[36;1m        jq '.labels[] | select(.name == "skip-notes")')�[0m
 �[36;1m    if [ -n "${label}" ]; then�[0m
 �[36;1m        echo "PR has skip-notes label; skipping"�[0m
 �[36;1m        exit 0�[0m
 �[36;1m    fi�[0m
 �[36;1mfi�[0m
 �[36;1mdiffinfo=$(curl --no-progress-meter \�[0m
 �[36;1m    -H "Accept: application/vnd.github+json" \�[0m
 �[36;1m    -H "Authorization: token ***" \�[0m
 �[36;1m    "https://api.github.com/repos/coreos/ignition/compare/ade39b435a73689a3a854037585d9dcb913fc86c...f72898152cef4e88f9aeeacbd487b6e527f12d22" |�[0m
 �[36;1m    jq '.files[] | select(.filename == "docs/release-notes.md")')�[0m
 �[36;1mif [ -z "${diffinfo}" ]; then�[0m
 �[36;1m    echo "Found no changes to docs/release-notes.md."�[0m
 �[36;1m    if [ -n "skip-notes" ]; then�[0m
 �[36;1m        echo "To ignore, add skip-notes label to PR."�[0m
 �[36;1m    fi�[0m
 �[36;1m    exit 1�[0m
 �[36;1mfi�[0m
 �[36;1mecho "Found change to docs/release-notes.md."�[0m
 shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
 ##[endgroup]
 Found no changes to docs/release-notes.md.
 To ignore, add skip-notes label to PR.
 ##[error]Process completed with exit code 1.

🧰 Additional context used

📓 Path-based instructions (2)

- **Naming**: PascalCase exported, camelCase unexported, snake_case filenames

📄 CodeRabbit inference engine (AGENTS.md)

Files:

butane/config/util/util.go

- **Formatting**: `gofmt` enforced (CI and `./test`)

📄 CodeRabbit inference engine (AGENTS.md)

Files:

butane/config/util/util.go

🔇 Additional comments (1)

butane/config/util/util.go (1)

159-159: LGTM!

📝 Walkthrough

Walkthrough

TranslateBytesYAML now uses the YAML decoder to parse intermediate JSON before converting the configuration to YAML. This prevents large numeric values from being emitted in scientific notation for OpenShift MachineConfigs.

Changes

YAML translation

Layer / File(s)	Summary
Intermediate configuration decoding `butane/config/util/util.go`	`TranslateBytesYAML` uses `yaml.Unmarshal` instead of `json.Unmarshal` before YAML re-encoding.

Estimated code review effort: 1 (Trivial) | ~3 minutes

Merge Risk: ⚪ Minimal · up to f7289

This localized change switches YAML unmarshalling to preserve numeric values more accurately; no actionable merge-blocking risk remains beyond normal checks and review.

Suggested reviewers: prestist

🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change addresses issue `#2309` by replacing JSON unmarshalling with YAML unmarshalling to preserve large integer MiB values and prevent scientific notation in generated MachineConfigs.
Out of Scope Changes check	✅ Passed	The pull request changes one relevant unmarshalling call and contains no unrelated code changes.
Commit Message Convention	✅ Passed	PASS. The PR contains one non-merge commit. Its subject is `butane/util: use yaml instead of json for unmarhal`: it has a subsystem, the description starts with lowercase `use`, uses imperative mood, …
Title check	✅ Passed	The title describes the YAML-for-JSON unmarshalling change, uses the required subsystem prefix, uses lowercase wording after the colon, and uses imperative mood. The description contains a typo in “un…
Description check	✅ Passed	The description directly explains the YAML unmarshalling change, its effect on numeric formatting, and the remaining test and release note work.

Full details: Commit Message Convention

Explanation

PASS. The PR contains one non-merge commit. Its subject is butane/util: use yaml instead of json for unmarhal: it has a subsystem, the description starts with lowercase use, uses imperative mood, and has no trailing period. The commit is the only commit in the PR range.

Full details: Title check

Explanation

The title describes the YAML-for-JSON unmarshalling change, uses the required subsystem prefix, uses lowercase wording after the colon, and uses imperative mood. The description contains a typo in “unmarhal,” but remains clear.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-08-27T14:01:35Z

Binary size report (`bin/amd64/ignition`)

	Size
Base (`main`)	33MiB
PR (#2310)	33MiB
Delta	-60KiB (-0.18%)

angelcerveraroldan · 2026-08-27T14:07:40Z

Seems like the issue was coming from how we handle MCO.

We first generate the igntion json from the butane yaml, then we unmarshal said json, and re-marshal it in yaml.

In the step where we are unmarshal'ing the json igntion config, we lose the type information, and float64 is chosen by default for all numbers, which are later formatted with scientific notation.

It seems that the reason we have to do this whole back and forth is due to missing functionality in the yaml library we use - which has been deprecated for about a year an a half now. I wonder if its worth looking into using an alternative yaml library that's maintained. @prestist wdyt

prestist · 2026-08-31T17:35:05Z

Seems like the issue was coming from how we handle MCO.

We first generate the igntion json from the butane yaml, then we unmarshal said json, and re-marshal it in yaml.

In the step where we are unmarshal'ing the json igntion config, we lose the type information, and float64 is chosen by default for all numbers, which are later formatted with scientific notation.

What a fun bug to encounter.

It seems that the reason we have to do this whole back and forth is due to missing functionality in the yaml library we use - which has been deprecated for about a year an a half now. I wonder if its worth looking into using an alternative yaml library that's maintained. @prestist wdyt

So, what functionality were we missing that we are covering by doing the back and forth for clarity?

Also, I think first things first we should create a test that ensures no regression here in validate_test.go (I think thats the right place..). But after that we should absolutely look into an alternative in a separate pr, let's fix the bug and then have a pr specific for replacing our tooling.

angelcerveraroldan · 2026-08-31T18:25:05Z

Sounds good, I'll finish up adding the test and release notes to this PR and then I can take a look at other yaml libraries, tho I am not sure if swapping will be worth it or not.

So, what functionality were we missing that we are covering by doing the back and forth for clarity?

The missing feature from the current yaml library leading to that back and forth is that it the Ignition structs have a json tag but no yaml tag, and the current lib cannot be setup to use the json tag for the names.

So we cannot directly go from Ignition structs -> yaml (doing this would lead to incorrect names in the yaml file). Instead we generate the json, then unmarshal the json into a map[string]interface{}, and then turn this into yaml (it will now have the correct names, as defined by the json tags).

But after that we should absolutely look into an alternative in a separate pr

The benefits of changing library would more or less be that we are using a maintained library rather than an archived/deprecated one, the MCO generation is a little cleaner, and we can probably get better error reporting UX.

I can make a PoC and we can see if its worth changing or not.

angelcerveraroldan force-pushed the butane/fix-large-number branch from 148aff9 to f728981 Compare August 27, 2026 14:01

coderabbitai Bot approved these changes Aug 27, 2026

View reviewed changes

This was referenced Aug 29, 2026

butane/util: emit large MachineConfig integers as decimals #2311

Closed

butane/util: emit large MachineConfig integers as decimals #2312

Closed

Navigation Menu

Uh oh!

butane/util: use yaml instead of json for unmarhal - #2310

butane/util: use yaml instead of json for unmarhal#2310
angelcerveraroldan wants to merge 1 commit into
coreos:mainfrom
angelcerveraroldan:butane/fix-large-number

angelcerveraroldan commented Aug 27, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Aug 27, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 27, 2026

Uh oh!

angelcerveraroldan commented Aug 27, 2026 •

edited

Loading

Uh oh!

prestist commented Aug 31, 2026

Uh oh!

angelcerveraroldan commented Aug 31, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

angelcerveraroldan commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Aug 27, 2026

Binary size report (bin/amd64/ignition)

Uh oh!

angelcerveraroldan commented Aug 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

prestist commented Aug 31, 2026

Uh oh!

angelcerveraroldan commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

angelcerveraroldan commented Aug 27, 2026 •

edited

Loading

coderabbitai Bot commented Aug 27, 2026 •

edited

Loading

Binary size report (`bin/amd64/ignition`)

angelcerveraroldan commented Aug 27, 2026 •

edited

Loading

angelcerveraroldan commented Aug 31, 2026 •

edited

Loading